Repository navigation
fix(macOS): keep legacy Home visible when toggled - #11933
Conversation
|
Thanks — the reachability analysis and the fix both look right. Verified against head 69918d7: Code
The two red checks are not from this PR's code. Both jobs fail only on
So this head can't go green as-is. Two clean paths: a standalone format-only fix on main (cleanest, keeps this PR scoped), then rebase/re-run here; or re-apply the branch's own b7f4c09 reformat. Either unblocks this and every other desktop PR currently failing that lane. One product-visible note for when this leaves draft: the new advanced toggle "Use oldest Home theme" ( by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
macOS
positive-signal
|
Update since the earlier review on this head: the unblock path has landed on main. c934d60 applies the pinned swift-format fix to by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
69918d7 to
eb2c1d8
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bf42c0d7-4c16-4555-b71a-558baa31efb2) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2c1d8d72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if useLegacyHomeDesign && useOldestHomeDesign && !routesChatToPrimaryShell { | ||
| legacyHome | ||
| } else { | ||
| redesignedHome |
There was a problem hiding this comment.
When “Use old Home design” is enabled and the new “oldest” toggle remains at its default false, this branch now renders redesignedHome while its behavior still treats any useLegacyHomeDesign == true state as the widgets-based Home. Consequently consumePendingMainChatOpenRequest() consumes and discards “Continue in Omi” requests, the homeStage* automation actions return early, homeMode is reported as unavailable, and focusing the hub ask field does not transition to chat. Derive these guards from the selected HomeDesignPresentation (or an isOldestLegacy value) rather than the outer preference flag.
AGENTS.md reference: desktop/macos/AGENTS.md:L350-L352
Useful? React with 👍 / 👎.
| maxWidth: CGFloat = ChatComposerLayout.contentLaneMaxWidth | ||
| ) -> NSSize { | ||
| NSSize( | ||
| width: min(size.width, visibleFrame.width, maxWidth), |
There was a problem hiding this comment.
After this change permits resizing the shell up to the display width, any user-selected width above the 900-point chat lane is still reduced to 900 here when the remembered frame is used. Thus widening the redesigned or oldest Home is lost after dismissing and summoning the shell, moving it through a summon to another display, or recreating the window, despite ShellFrameMemory recording the wider frame and landingFrame explicitly promising to preserve a user-widened size. The obsolete pre-hug clamp needs to distinguish migrated frames from widths that are now valid, or be removed.
Useful? React with 👍 / 👎.
| /// Pins the shell between the destination minimum and the visible display frame. | ||
| private static func pinShellWindowSizeLimits(_ window: NSWindow, resizeFrame: Bool = true) { | ||
| let minimumContentSize = DesktopWindowLayoutPolicy.minimumContentSize | ||
| let maximumContentSize = NSSize( | ||
| width: DesktopWindowLayoutPolicy.maximumContentWidth, | ||
| height: 10_000) | ||
| let maximumContentSize = DesktopWindowLayoutPolicy.maximumContentSize(for: window) |
There was a problem hiding this comment.
The maximum is now derived from window.screen, but it is only sampled during initial enforcement, activation/navigation, and didResize; moving an already-wide window from a large monitor to a smaller one emits didMove/screen-change notifications without necessarily resizing it. The window therefore retains the larger display's maxSize and can remain wider or taller than the new visible frame, while the existing stranded-frame recovery accepts it as long as 320×240 points overlap a screen. Re-pin and clamp on window-screen/display-parameter changes as well as resize.
Useful? React with 👍 / 👎.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Following up on the earlier reviews of 69918d7: the rebase landed cleanly and clears the last blocker, so this is now in mergeable shape.
What I verified on eb2c1d8:
- The functional patch is byte-identical to the previously verified one — the only changes vs 69918d7 are the rebase onto current main and the trailing auto-format commit, so the file-by-file analysis from the earlier review carries over unchanged.
- The format-lint blocker is gone at the root: c934d60 (main's pinned swift-format fix for
ConversationDisplayStateTests.swift) is now an ancestor of this head, and the branch's auto-format commit resolves to main's exact bytes — the PR's net diff no longer touches that file at all. Both previously red desktop jobs are green on this head, along with the release compile and the static/test contracts. - The commit trail stays clean and scoped: 77d42b7 grounds legacy Home in the page glass lane, e54859e restores reachability of all three Home presentations with its failure-class declared, and eb2c1d8 is pure formatting fallout from the rebase.
Everything that made this a good fix is still here: the router-resolved panel-ownership decision instead of the hardcoded .dashboard self-ownership (the actual root cause), the two older Home surfaces reachable and grounded through PageGlassLane, the display-sized window maximum with the 900pt readable cap moved to the content lane, and tests covering each polarity the change introduced. The "Use oldest Home theme" toggle nested under the existing legacy-design setting completes the three-presentation story coherently rather than opening a new direction.
Approving — thanks for the patience through the rebase round-trips.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
removed
the
positive-signal
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Owner-override (desktop bug fix): legacy Home transparency fix, CI green, CLEAN, well-tested (PageGlassLane ownership routing).
ShellWindowChrome intentionally leaves the window clear, but the legacy DashboardPage was still classified as owning its own panels. When useLegacyHomeDesign selected that route, no PageGlassLane was mounted and the window exposed the clear shell. Pass the flag into the lane policy so legacy DashboardPage receives the shared panel ground while the modern query-shell Home keeps its existing self-owned panels. Verification: focused PageGlassLane, glass, shell chrome, and chat-first tests passed; debug build, SwiftLint, desktop test-quality, targeted Swift-format, and make preflight passed. The full Desktop suite had one unrelated flaky RewindCaptureExclusionGenerationTests failure whose isolated rerun passed. Failure-Class: none
Route the legacy preference to the redesigned hub and add an explicit oldest Home theme. Pass the resolved glass ownership decision into PageGlassLane, lift the window maximum to the visible display, and preserve the new QueryShell glass lane. Failure-Class: FC-transparent-top-level-window-occlusion
eb2c1d8 to
63fd3a5
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f6543c37-c4c0-43cd-b67f-2595b0e516fe) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63fd3a5901
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if useLegacyHomeDesign && useOldestHomeDesign && !routesChatToPrimaryShell { | ||
| legacyHome | ||
| } else { | ||
| redesignedHome |
There was a problem hiding this comment.
When “Use old Home design” is enabled but “Use oldest Home theme” is not, this branch renders redesignedHome, whose app and connector actions present homeConnectSheetOverlay; however, the three legacySelected* bindings still become non-nil whenever useLegacyHomeDesign is true, so the same selection also presents a dismissableSheet. Opening an app, import connector, or export destination therefore mounts two modal implementations over each other, causing duplicate content/scrims and duplicate onAppear side effects such as app-detail analytics. Gate the legacy bindings with the resolved .oldestLegacy presentation rather than the outer preference.
AGENTS.md reference: AGENTS.md:L46-L46
Useful? React with 👍 / 👎.
Six commits, no conflicts. Nineteen of the forty files are Swift desktop (legacy Home visibility, BasedHardware#11933, plus the v0.12.209 changelog). The rest is upstream's Cloud Run / GCP deploy lane: converging automatic dev deploys (BasedHardware#12019), a preflight that can see past the metrics sidecar (BasedHardware#12080), and the GMP secret annotation written with a project number (BasedHardware#12081). Post-merge audit (ADR-0029/0030): nothing to resolve. ports untouched — no file under database/store/, utils/{auth,vector,object_store}/, _client.py, or the notification transport is in the diff at all fidelity the four new backend modules (attach_cloud_run_gmp_sidecar, firebase_release_probe_token, preflight-cloud-run-deploy, runtime_env_validation/workflows) introduce no raw persistence and no firebase_admin.auth — they are deploy tooling, not request-path code guards 9/9 exit 0, including the env-parity guard: upstream declared no new runtime variable this time Worth noting for our own backlog: BasedHardware#12080 and the GMP sidecar attach script are the Cloud Run mechanism behind PROMETHEUS_SIDECAR_PORT, which the previous merge wrote off as a shape we do not deploy. Upstream building on it confirms the write-off was the right call rather than a gap on our side. Verification contract lane 55 files, 1083 passed, 78 skipped (live-tier only), 0 failed full sweep 1027 files in 3m19s, FAIL set identical to the pre-merge baseline upstream's five new backend test files run and pass under our runner (8 + 9 + 13 + 18 + 49), and their two new .github guard suites pass (37 + 67 subtests)
* fix(desktop): ground legacy home in page glass lane ShellWindowChrome intentionally leaves the window clear, but the legacy DashboardPage was still classified as owning its own panels. When useLegacyHomeDesign selected that route, no PageGlassLane was mounted and the window exposed the clear shell. Pass the flag into the lane policy so legacy DashboardPage receives the shared panel ground while the modern query-shell Home keeps its existing self-owned panels. Verification: focused PageGlassLane, glass, shell chrome, and chat-first tests passed; debug build, SwiftLint, desktop test-quality, targeted Swift-format, and make preflight passed. The full Desktop suite had one unrelated flaky RewindCaptureExclusionGenerationTests failure whose isolated rerun passed. Failure-Class: none * fix(desktop): restore reachable Home presentations Route the legacy preference to the redesigned hub and add an explicit oldest Home theme. Pass the resolved glass ownership decision into PageGlassLane, lift the window maximum to the visible display, and preserve the new QueryShell glass lane. Failure-Class: FC-transparent-top-level-window-occlusion * style(desktop): auto-format ConversationDisplayStateTests after rebase --------- Co-authored-by: Max Carter 祁明思 <undivisible@users.noreply.github.com>
Root cause
ShellWindowChromeintentionally leaves the top-level window clear and non-opaque. The modern query-shell Home owns its own glass, but whenuseLegacyHomeDesignselectedDashboardPage,PageGlassLanePolicystill classified the dashboard as self-contained. The legacy dashboard therefore received no panel ground and exposed the transparent shell.Fix
Make
PageGlassLanePolicyreceive the resolved Home surface ownership decision fromDesktopHomeView. The modern query-shell Home and Rewind keep their existing self-owned panel behavior; the redesigned and oldest legacy Home surfaces mount through the sharedPageGlassLane, which supplies the visible panel ground. The Home switcher now reaches all three presentations, and the window maximum follows the visible display while the content lane remains 900pt. Tests cover both ownership polarities, the unknown-index route, all three presentations, and the display-sized maximum.Product invariants affected
Verification
xcrun swift build -c debug --package-path Desktop— passed.xcrun swift test --package-path Desktop --filter PageGlassLaneTests— 8 passed.xcrun swift test --package-path Desktop --filter QueryShellTests— 38 passed.xcrun swift test --package-path Desktop --filter DesktopWindowLayoutPolicyTests— 2 passed.xcrun swift test --package-path Desktop --filter ShellSummonTests— 26 passed.xcrun swift test --package-path Desktop --filter TopNavigationBarLayoutTests— 18 passed.xcrun swift test --package-path Desktop --filter ConversationDisplayStateTests— 10 passed.com.omi.omi-legacy-homewas built, installed, launched, and exercised through the automation bridge with a synthetic local Auth/Firestore profile. Screenshots cover QueryShell Home, redesigned hub, oldest Home, and visible-frame fill.Failure-Class: FC-transparent-top-level-window-occlusion
Line-Count-Exception: desktop/macos/Desktop/Sources/MainWindow/DesktopHomeView.swift | 1636 -> 1637 | Rebase onto current main kept Memory-hub destination glass ownership on the same PageGlassLane call; one extra argument is cheaper than splitting the router.
Note
Medium Risk
Touches main-window glass ownership and AppKit min/max sizing, so a wrong flag or max-size can make Home transparent or let the window fill the display. No auth or data-path changes.
Overview
Stops the older Home designs from rendering onto the wallpaper.
PageGlassLanenow takes a router-suppliedhomeOwnsItsPanelsflag, so QueryShell Home and Rewind keep their own glass while redesigned and oldest Home get the shared panel.Adds a nested Use oldest Home theme setting under legacy Home, resolved through
HomeDesignPresentation(query shell / redesigned hub / oldest widgets-and-chat). Older Home backgrounds stay clear so the lane is the ground.The shell’s max size follows the visible display instead of the 900pt readable lane. Summoned windows still hug
ChatComposerLayout.contentLaneMaxWidth. Tests cover both ownership polarities, all three presentations, and display-sized maxima.Reviewed by Cursor Bugbot for commit 63fd3a5. Configure here.